Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CURA-10722] Add 'additional settings providers' (Engine Plugins) #890

Merged

Conversation

rburema
Copy link
Member

@rburema rburema commented Jul 26, 2023

Plugins should now be able to add settings. (But not append enum-values to existing ones, that's another ticket.)

  • (De)serialization works, including load/save.
  • Keys are tagged/'namespaced' with provider type (just a plugin for now), unique id, version.
  • Will be sent to engine (except you have to demangle the tagged/namespaced name a bit).
  • Setting-formulas work (except for taking values of another plugin).
  • Can add to an existing category, or create a new one.

See (now) also the accompanying front-end PR here: Ultimaker/Cura#16305

Since they're loaded on top instead of going through the normal inheritance system, we need to merge/add them to each definition-container. This can't happen during load from file either, since most times, it's loaded from the cache-db instead. Instead, load the extra settings into the Definition ones each time one is loaded. If this turns out to be slow, we could pre-parse the json into (proto) SettingsDefinitions beforehand maybe, but it's not clear how much that is the actual bottleneck and how much code would need to be either duplicated or upended.

part of CURA-10722
Needed to use underscores instead of :: so that it would work as python variable names.

part of CURA-10722
When saving to cfg (which is in the zipped 3mf file), the variables are forced to lower case. If a plugin-ID contains capital letters, it would not be able to match the settings it saved and the setting loaded otherwise.

part of CURA-10722
Copy link
Member

@jellespijker jellespijker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; As we discussed during the stand-up we will add functionality to the plugin system as a whole (both front-end and curaengine_plugins) which will allow to add settings by simply specify a path to a json file. Which will have setting entries such as they are defined in fdmprinter.def.json

UM/Settings/DefinitionContainer.py Outdated Show resolved Hide resolved
UM/Settings/DefinitionContainer.py Outdated Show resolved Hide resolved
@jellespijker
Copy link
Member

Also pinging @fieldOfView because I think he also could have some valuable insights on this PR and at the very least FYI

@github-actions
Copy link

github-actions bot commented Jul 28, 2023

Unit Test Results

2 406 tests  +2   2 391 ✔️ +2   25s ⏱️ -3s
       1 suites ±0        15 💤 ±0 
       1 files   ±0          0 ±0 

Results for commit d0c8ca9. ± Comparison against base commit 6a8c6c0.

♻️ This comment has been updated with latest results.

You could still override the getAdditionalSettingDefinitions method like before to get the same result as previous, but you can now also just put a path in the definition_file_paths variable to load from there instead.

part of CURA-10722
Not all of them have to be for engine-plugins after all.

part of CURA-10722
@fieldOfView
Copy link
Contributor

Also pinging @fieldOfView

'sup?

Hmm, I don't quite get why this would be necessary. Many of my plugins are adding settings that eventually also get sent to CuraEngine (which happily ignores them), then to be used in postprocessing. It is all a bit hacky (private method access), but has been working very well for years.

@rburema
Copy link
Member Author

rburema commented Jul 28, 2023

@fieldOfView -- If you don't see the need to change for your own plugins, that's fine. We're basically using the same mechanism anyway, so we won't clash.

Since (other) people are (we hope) going to be writing engine-plugins (also), there is additional need to make a proper (non-hacky) way to define additional settings for engine plugins. -- We may trust you with our internals, but that's not necessarily the case for every developer out there.

Also to us these settings do not feel quite the same as plugin preferences, and we'd like the possibility of defining values for them in profiles (for ex.; a 'weird printer' plugin that comes with an engine part and a bunch of profiles for example).

Additionally, different plugins might have the same setting-definition name added. We tag/'sort-of-namespace' the settings internally so that they at least don't destabilize the system. A similar story for different versions of the same plugin that might have tweaked their setting-definition meanings.

Copy link
Member

@nallath nallath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from my other review comments; We need unit tests for this.

UM/Settings/DefinitionContainer.py Show resolved Hide resolved
UM/Settings/AdditionalSettingDefinitionAppender.py Outdated Show resolved Hide resolved
The prepend method was only ever used by the Appender anyway. This is simpler.

part of CURA-10722
@rburema rburema dismissed nallath’s stale review August 1, 2023 14:25

performed requested changes :-)

part of CURA-10722
@casperlamboo casperlamboo merged commit 8ed0731 into CURA-10475_engineplugin Aug 3, 2023
8 checks passed
@casperlamboo casperlamboo deleted the CURA-10722_plugins_can_has_settings branch August 3, 2023 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants